fix insufficient flushing on ARMv8.2+ #5257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ARMv8.2 separates flush-for-coherency from flush-for-persistency as two instructions: DC CVAC and DC CVAP. It's the latter that is of interest to us, and it is up to the processor to know where the Point of Persistency happens to be. The documentation is very clear, and this PIRL talk dispels all doubt: https://www.youtube.com/watch?v=8QAuN8CL5Zg On the other hand, I found no clear answer whether DC CVAC might be enough on some pre-8.2 hardware. There's no other option though (CVAC is the heaviest flush available). This question is probably moot as pre-8.2 CPUs, while ubiquitous in dev boards, are gone in datacenter gear.
Codecov Report
@@ Coverage Diff @@
## master #5257 +/- ##
==========================================
- Coverage 72.95% 72.91% -0.04%
==========================================
Files 187 187
Lines 29154 29129 -25
==========================================
- Hits 21269 21240 -29
- Misses 7885 7889 +4 |
pbalcer
reviewed
Jul 2, 2021
| @@ -38,10 +71,15 @@ pmem2_arch_init(struct pmem2_arch_info *info) | |||
| LOG(3, NULL); | |||
|
|
|||
| info->fence = memory_barrier; | |||
| info->flush = flush_dcache; | |||
| if (is_dc_pop_available()) | |||
There was a problem hiding this comment.
In the future, this should also include a check for ARM's eADR equivalent. But for now, it's fine.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
You need to use Point-of-Persistency flushes, defined specifically for our use case. This instruction has been split so Point-of-Coherency could be made lighter.
The documentation is clear here, and it's an obvious data loss bug, but no one of us had access to an ARMv8.2 machine to test before. No help has been offered by ARM (in stark contrast to IBM with ppc) thus there's no official support for PMDK on arm64 — but not flushing is severe enough to be fixed even in experimental code.
This change is